feat: add a reason field before PAM account access#6096
feat: add a reason field before PAM account access#6096carlosmonastyrski wants to merge 3 commits intomainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 264d397100
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -0,0 +1,25 @@ | |||
| import { Knex } from "knex"; | |||
|
|
|||
There was a problem hiding this comment.
Ignore comment placement.
Can we limit the amount of text to a maximum? How do you think it is necessary to show the full reason?
| </h2> | ||
| <p className="max-w-sm text-center text-xs text-mineshaft-400"> | ||
| {isReasonRequired | ||
| ? "This account's policy requires a reason for access. The reason will be stored with the session for audit purposes." |
There was a problem hiding this comment.
no need to mention "stored with the session" - we don't want to expose the internal structures, can just say will be stored for audit purposes.
| <p className="max-w-sm text-center text-xs text-mineshaft-400"> | ||
| {isReasonRequired | ||
| ? "This account's policy requires a reason for access. The reason will be stored with the session for audit purposes." | ||
| : "Optionally provide a reason for this session. The reason will be stored with the session for audit purposes."} |
There was a problem hiding this comment.
no need to mention "stored with the session" - we don't want to expose the internal structures, can just say will be stored for audit purposes.
| lastRotationMessage: z.string().nullable().optional(), | ||
| rotationStatus: z.string().nullable().optional() | ||
| rotationStatus: z.string().nullable().optional(), | ||
| requireReason: z.boolean().default(false) |
There was a problem hiding this comment.
this is always false on create/update responses since only getById computes it? can we also compute on create and update? since we are returning policy info already
| @@ -0,0 +1,25 @@ | |||
| import { Knex } from "knex"; | |||
|
|
|||
| import { TableName } from "../schemas"; | |||
There was a problem hiding this comment.
Ignore comment placement.
Can you update the docs as well? Should be a quick one.
3 participants
Context
Add a new reason field on PAM sessions with a possible setting to enforce the field on user access.
Screenshots
Steps to verify the change
Type
Checklist
type(scope): short description(scope is optional, e.g.,fix: prevent crash on syncorfix(api): handle null response).